Remove reference counting from InboundMessage and make it Releasable #126138
Conversation
This is pretty much the mirror issue to elastic#122981, solving the same problem on the inbound side. We should also eagerly release buffers after deserialization. Especially for bulk requests this could become quite helpful.
original-brownbear
added
>non-issue
:Distributed Coordination/Network
elasticsearchmachine
added
the
Team:Distributed Coordination
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
| Releasables.assertOnce(message.takeBreakerReleaseControl()) | ||
| ); | ||
| try { | ||
| try (message) { |
There was a problem hiding this comment.
Admittedly annoying to spread the release logic across so many spots but in the end that is precisely what we need/want, release right where we deserialize. (If we want this cleaner we need to deserialise in fewer spots I guess?)
There was a problem hiding this comment.
Yeah this is the same thing as refcount-negative methods: we need to find a way to make the fact that the method closes one of its arguments explicit at the call site.
There was a problem hiding this comment.
Or alternatively (and I'm starting to think this is more practical), any method that consumes an object like that must either pass it on elsewhere (and do so exactly once) or release it.
Just documenting that something closes the argument still leaves a lot of complexity around having to check that the last receiver of the instances is one of those closing-enabled methods and that other methods don't start holding on to instances doesn't it?
There was a problem hiding this comment.
The trouble is I don't think this is (or could be made) true of every method that takes an argument which represents nontrivial resources. We need to support both.
There was a problem hiding this comment.
... still leaves a lot of complexity around having to check...
sure, it's not a fully automated answer (i.e. a proper borrow-checker) but at least it reduces those checks to method-local things rather than having to reason about things up and down the stack some arbitrary distance.
DaveCTurner
left a comment
DaveCTurner
left a comment
There was a problem hiding this comment.
I pushed a commit with the comments at the call sites that I'd like to see, wdyt?
| } finally { | ||
| aggregated.decRef(); | ||
| if (aggregated != null) { | ||
| // TODO doesn't messageHandler auto-close always? |
There was a problem hiding this comment.
looking at messageHandler (i.e. TcpTransport#inboundMessage) I think we close on all paths already, and catch everything anyway, so this seems redundant?
There was a problem hiding this comment.
Right ... let me fix that right up :)
| final T request; | ||
| try { | ||
| request = reg.newRequest(stream); | ||
| message.close(); // eager release message to save heap |
There was a problem hiding this comment.
Could we drop this one line from this PR and do it in an immediate follow-up? That way we have one commit that replaces refcounting with one-shot closing without changing semantics and then another that reduces the lifecycle.
There was a problem hiding this comment.
++ makes sense dropped it here
DaveCTurner
left a comment
DaveCTurner
left a comment
There was a problem hiding this comment.
LGTM with suitable fixes to the title and OP.
original-brownbear
added
the
auto-backport
original-brownbear
changed the title
|
Thanks David! Follow-up incoming :)! |
💔 Backport failed
You can use sqren/backport to manually backport by running |
Follow-up to elastic#126138. We can now release requst bytes directly after deserialization. Also, request instances need not go through a ref-counting cycle when forking, removing some contention from transport threads.
|
Follow-up in #126998 :) |
Follow-up to #126138. We can now release requst bytes directly after deserialization. Also, request instances need not go through a ref-counting cycle when forking, removing some contention from transport threads.
3 participants
There is no actual need t oreference count InboundMessage instances, their lifecycle is completely linear so we can simplify it away. This saves a little work directly but more importantly, it enables more eager releasing of the underlying buffers in a follow-up.